chore: remove legacy wallet #18749
Merged
chore: remove legacy wallet #18749
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
J-Son89
commented
Feb 7, 2024
| (js-delete response-js "accounts") | ||
| (rf/merge cofx | ||
| (process-next response-js sync-handler) | ||
| (wallet/update-wallet-accounts (types/js->clj accounts)))) |
There was a problem hiding this comment.
on second thoughts, perhaps I have removed too much here, and it's just the wallet cofx I need to remove 🤔
There was a problem hiding this comment.
Yeah, we probably need to revisit this to move (or create a clone) of the update-wallet-accounts method and update (dispatch events) the assets (tokens and collectibles) if any new account is added.
Jenkins BuildsClick to see older builds (29)
|
| (fn [accounts] | ||
| (wallet.utils/get-default-account accounts))) |
There was a problem hiding this comment.
Should we move the methods get-default-account and default-address method from legacy.status-im.wallet.utils to status-im.wallet.utils ns ?
There was a problem hiding this comment.
We can just add it in a separate pr, there's enough noise in this one 😰
| (js-delete response-js "accounts") | ||
| (rf/merge cofx | ||
| (process-next response-js sync-handler) | ||
| (wallet/update-wallet-accounts (types/js->clj accounts)))) |
There was a problem hiding this comment.
Yeah, we probably need to revisit this to move (or create a clone) of the update-wallet-accounts method and update (dispatch events) the assets (tokens and collectibles) if any new account is added.
Comment on lines
14
to
16
| (when (and is-connected? | ||
| (or (not= (count (get-in db [:wallet-legacy :accounts])) | ||
| (count (get db :profile/wallet-accounts))) | ||
| (wallet/has-empty-balances? db))) | ||
| (wallet/update-balances nil nil)))) | ||
| (count (get db :profile/wallet-accounts)))))))) |
There was a problem hiding this comment.
I guess this whole when is not needed at this moment as we don't store data in :wallet-legacy or :profile/wallet-accounts.
J-Son89
commented
Feb 7, 2024
| @@ -49,19 +49,6 @@ | |||
| [legacy.status-im.ui.screens.sync-settings.views :as sync-settings] | |||
| [legacy.status-im.ui.screens.wakuv2-settings.edit-node.views :as edit-wakuv2-node] | |||
There was a problem hiding this comment.
need to verify if any of these screens are still accessible
J-Son89
force-pushed
the
jc/remove-legacy-wallet
branch
from
c7005ee to
312576a
Compare
J-Son89
commented
Feb 8, 2024
| (log/debug "[keycard response succ] export-key") | ||
| (re-frame/dispatch [:keycard.callback/on-export-key-success | ||
| response])) | ||
| (js/alert "feature no longer supported")) |
There was a problem hiding this comment.
discussed this @cammellos -
as I understand Keycard is not hooked up at all and we soon will need to reimplement it with the new ui.
For that reason these changes are "safe" - I will add a follow up pr to remove the keycard related pieces completely.
There was a problem hiding this comment.
(in case the component/screen is still accessible) Maybe alert "feature under development" instead of "feature no longer supported" which can imply we have no intention of supporting Keycard in the future
There was a problem hiding this comment.
yeah good point, I think the screen is no longer viewable but to be on the safe side I will change it to that.
J-Son89
commented
Feb 8, 2024
| @@ -144,7 +143,7 @@ | |||
| :container-style styles/input-container-style | |||
| :accessibility-label :dapp-url-input | |||
| :return-key-type :go}] | |||
| [components/separator-dark] | |||
| [react/view {:style {:margin-top 8}}] | |||
There was a problem hiding this comment.
I removed the separator component and just added in a quick gap component, not sure how it looks but does any one know if we really care?
When is browser scoped for the redesign?
@cammellos @mohsen-ghafouri @Parveshdhull any ideas? 🤔
There was a problem hiding this comment.
does any one know if we really care?
At this stage probably not
not sure how it looks
It should be ok, but if you want to be sure, just use the same styles as the separator-dark component itself.
{:height 1 :background-color (colors/alpha colors/black 0.1)}
There was a problem hiding this comment.
true true, thanks @Parveshdhull ! 🙏
J-Son89
commented
Feb 8, 2024
| :on-press #(re-frame/dispatch [:bottom-sheet/show-sheet-old | ||
| {:content (fn [] [sheets/accounts-list :from | ||
| ::ens/change-address])}])}])) | ||
| :on-press #(js/alert "funcitonality no longer supported")}])) |
There was a problem hiding this comment.
related to ens name service -
Do I need to take caution with this still being intact? 🤔
perhaps it's better to remove account switcher completely? maybe ens support can be taken out of old ui?
J-Son89
commented
Feb 8, 2024
| :style {:background-color colors/gray-lighter | ||
| :border-radius 16} | ||
| :image-style {:width 24 :height 24})] | ||
| [react/view] |
There was a problem hiding this comment.
afaik signing screens can be removed later on too as they are just related to wallet? 🤔
J-Son89
commented
Feb 8, 2024
| (let [display-symbol (wallet.utils/display-symbol token) | ||
| fee-display-symbol (wallet.utils/display-symbol (tokens/native-currency chain))] | ||
| (let [display-symbol nil | ||
| fee-display-symbol nil] |
There was a problem hiding this comment.
Why make it nil? If the screen is still accessible shouldn't we keep it as it is, otherwise remove it completely?
There was a problem hiding this comment.
I will be removing signing in a pr after this. The old wallet is completely non functional now so the user should not get to a signing screen with the old wallet.
J-Son89
commented
Feb 8, 2024
| @@ -38,7 +38,7 @@ | |||
| (or owned free) | |||
| (re-frame/dispatch [:stickers/install-pack id]) | |||
| not-enough-snt? nil | |||
| :else (re-frame/dispatch [:stickers/buy-pack id]))} | |||
| :else (fn [] (js/alert "feature no longer supported")))} | |||
There was a problem hiding this comment.
Is this okay to do? We have sticker support in the new UI for MVP afaik.
right @Parveshdhull ?
cc @cammellos
There was a problem hiding this comment.
Hi @J-Son89, Thank you for pinging.
We do have stickers in the design file but they are not implemented yet.
I don't know if we will use this code or not, but maybe for now just comment out for future reference. wdyt?
(Means event part, ui is already inaccessible)
There was a problem hiding this comment.
Thanks for the help @Parveshdhull! 🙏
Ok perfect, in that case this change is "safe" so. I will prefer to remove the code instead actually as we have git history for that very purpose and removing stale/commented out code leaves the codebase feeling cleaner and more maintained. However I will remove stickers in a follow up as I don't want this pr to grow in scope.
On a further point I think we should remove as much of the "legacy" folder as being unused as often when we are making "new" ui integrations there is some mixing of the legacy and new and then it becomes messy to maintain. I'll try make a push to remove more this month.
J-Son89
commented
Feb 9, 2024
| @@ -86,7 +85,7 @@ | |||
| (js/setTimeout | |||
| #(browser/share-link url) | |||
| 200))}] | |||
| [components/separator]]) | |||
| [react/view {:style {:margin-top 8}}]]) | |||
There was a problem hiding this comment.
updated to other designs as Parvesh suggested 👍
J-Son89
force-pushed
the
jc/remove-legacy-wallet
branch
from
312576a to
60fb189
Compare
J-Son89
requested review from
rasom,
flexsurfer,
mohsen-ghafouri,
briansztamfater,
OmarBasem,
vkjr,
Parveshdhull,
smohamedjavid and
a team
ulisesmac
approved these changes
Feb 13, 2024
OmarBasem
approved these changes
Feb 15, 2024
J-Son89
force-pushed
the
jc/remove-legacy-wallet
branch
from
58f4362 to
cb0847d
Compare
10% of end-end tests have passedFailed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (5)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
0% of end-end tests have passedFailed tests (47)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
|
Hi @J-Son89 ! CAn you please mention items/places needs to be tested from our side? |
|
@mariia-skrypnyk, sure. It is all of the pieces of code related to the old wallet UI, the old wallet signing page, the old stickers page and the old way of purchasing the ENS names. Afaik none of these pages are actually viewable anymore, so in that case we just need to make sure the app is working fine and a smoke check/regression test should be enough, especially with the new wallet. If some of these older screens are possible to navigate to then we might have to verify their behaviour and what is happening - in the case of purchasing ENS names I have been given the green light to remove these so I will do that in a follow up issue/pr. Perhaps some of the other members of QA know about some hidden way to view some of these old views but I could not access them. |
96% of end-end tests have passedFailed tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Hi @J-Son89! No old wallet UI found on my side. Smoke testing showed only known reported issues. |
siddarthkay
added a commit
that referenced
this pull request
Feb 27, 2024
fixes #19012 ### Summary The legacy wallet code was removed here -> #18749 as part of that purge we also cleaned up re-frame subscriptions for wallet push notifications. Since the new wallet is under active development we would not need this toggle in settings just yet. This commit fixes the app crashing on settings UI. ## Platforms - Android - iOS
siddarthkay
added a commit
that referenced
this pull request
Feb 27, 2024
fixes #19012 ### Summary The legacy wallet code was removed here -> #18749 as part of that purge we also cleaned up re-frame subscriptions for wallet push notifications. Since the new wallet is under active development we would not need this toggle in settings just yet. This commit fixes the app crashing on settings UI. ## Platforms - Android - iOS
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Starts working on: #18559
Note:
This pr got a bit out of hand - Happy to do this in smaller chunks if anyone prefers this. However afaik none of these screens are connected to the app at all anymore. There might be one or two small features that are still possible to see.
The things this pr removes is:
there is also some minor parts related to keycard and signing that become affected here but these are not currently used in the app and we will be implementing both soon in the new mobile ui so they are safe to remove. However this will be done in a follow up pr!